Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat / Hero and HeroShelf improvements #642

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

royschut
Copy link
Collaborator

@royschut royschut commented Nov 14, 2024

Description

This PR improves the Hero and HeroShelf and removes Card shadow:

  • Fade out the background incrementally based on scroll position. This brings a much smoother scrolling experience.
  • Add swiping gesture to HeroShelf for all breakpoints. Previously swiping was only possible for mobile devices, but on tablets or landscape mobile it is needed as well to navigate between slides. I have tried simplifying the code by merging the two implementations, leaving only style differences between mobile and the rest.
  • Some minor style updates to HeroShelf: a bit more height on tablet, better support for mobile landscape and respecting the safe-area better (useful for non-web platforms).
  • Make the Hero fixed as well, and add the same fade behaviour.
  • Remove the Card box-shadow. Since we've re-introduced support for a light background, the card shadow seemed a bit overdone, or out-dated even in some opinions (have a look here: https://vd-ott.com/?app-config=kcm2oevm). We figured that the card shadow could be left out entirely, as it was once introduced mainly because of the blur background, which was removed quite some time ago.

Steps completed:

According to our definition of done, I have completed the following steps:

  • Acceptance criteria met
  • Unit tests added
  • Docs updated (including config and env variables)
  • Translations added
  • UX tested
  • Browsers / platforms tested
  • Rebased & ready to merge without conflicts
  • Reviewed own code

Copy link

github-actions bot commented Nov 14, 2024

Visit the preview URL for this PR (updated for commit ed17096):

https://ottwebapp--pr642-feat-hero-shelf-upda-dw7onm3d.web.app

(expires Sat, 14 Dec 2024 13:44:56 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

@royschut royschut marked this pull request as ready for review November 14, 2024 12:02
@royschut royschut changed the title Feat / hero shelf update Feat / Hero and HeroShelf improvements Nov 14, 2024
refactor(home): align tablet with mobile
refactor(home): make swipeslider more stable

refactor(home): optimize heroshelf landscape
@langemike
Copy link
Collaborator

langemike commented Nov 14, 2024

These changes add a lot of value! Swiping works better, indeed. I think we're on the right track, but I noticed some issues that are related to different (smartphone) resolutions.

There are 3 styling-related issues. These weren't there previously.

  1. The Hero title can get cut-off on top.

  2. The buttons look crammed now (too narrow button container)

  3. Also in some specific resolutions, the Start Watching button doesn't show, as can be seen here:
    Screenshot 2024-11-14 at 13 18 07

And I also noticed 2 issues in the scrolling (fade to transparent) behaviour:

  1. It should finish its transition when the hero is out of view. But it happens around the second regular shelf
  2. It doesn't fade completely away

These happen on certain resolutions such as 2340x1080 (on my phone). On other resolution, I see it works as intended.

I tested this on the deploy preview.

Video recording showcasing styling issues 1 & 2 and scrolling issues 1 & 2.

phone.mp4

Comment on lines +22 to +27
/* stylelint-disable length-zero-no-unit */
--safe-area-top: 0px;
--safe-area-right: 0px;
--safe-area-bottom: 0px;
--safe-area-left: 0px;
/* stylelint-enable length-zero-no-unit */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come using 0 is a problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chrome doesn't respect it within a calc function, and ignores setting the shelf height.


const slideTo = (toIndex: number) => {
useScrolledDown(50, isMobile ? 200 : 700, (progress: number) => {
if (posterRef.current) posterRef.current.style.opacity = `${Math.max(1 - progress, isMobile ? 0 : 0.1)}`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also fade away the pagination?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find it necessary anymore. Before they were also fixed, but they scroll away now.

refactor(home): minor style updates

refactor: minor update
containerRef.current.style.transition = 'transform 0.2s ease-out';
containerRef.current.style.transform = `translateX(100%)`;
} else if (direction === 'right') {
} else if (isSwipeAnimation && direction === 'right') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it look nicer for tablets and larger screen sizes to decrease the left/right offset and use opacity as well? It works great now, but the slide animation distance is pretty big (opinionated ofc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you mean the animation after a swipe gesture (because the normal slide hasn't changed). The thing is: when you drag the metadata over the screen, then let go, you want the gesture to be continued similarly. So I cannot really change the opacity, you expect it to be opaque. Same for the offset, it comes from outside of the screen. I could only update the transition perhaps?

@royschut
Copy link
Collaborator Author

royschut commented Nov 14, 2024

There are 3 styling-related issues. These weren't there previously.

Thanks for the thorough testing @langemike! My last commit fixes the issues you mentioned.
About the scrolling fade: I have updated the end of the scroll/opacity window slightly, but if I change it too much, I find the transition a bit too sudden. You're now previewing on landscape, which is quite a specific viewport, I prefer to focus on more used viewports (without introducing too many variations). I have also removed the transition, as Christiaan suggested, which might help in your experience. How does the latest update feel?
Leaving 0.1 opacity is a choice by the way.

@langemike
Copy link
Collaborator

langemike commented Nov 14, 2024

How does the latest update feel?

Better but the video details section does a weird jump now during the slide animation.

Leaving 0.1 opacity is a choice by the way.

Ok. Good to know!

I think the fade out to 0.1 still happens quite late. But I can live with that.

updated video recording:
https://github.com/user-attachments/assets/df89d9a8-8bd5-40a4-86ed-44551699325b

ps. This can also be reproduced in Chrome using "responsive mode" using 2340x1080 or compatible screen size.

@royschut
Copy link
Collaborator Author

I think the fade out to 0.1 still happens quite late. But I can live with that.

It's a compromise on mobile landscape, to bring the best experience on more common viewports (mobile portrait, tablet and desktop). The only alternative I see is introducing more variation for the scroll/fade position, but since it is done from javascript, I prefer to keep it simple. But open to suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants